feat(state): add headers to comply with LDP specification #6917
Conversation
features/main/ldp_resources.feature
Outdated
| When I add "Content-Type" header equal to "application/ld+json" | ||
| And I send a "GET" request to "/dummy_get_post_delete_operations" | ||
| Then the header "Accept-Post" should be equal to "text/turtle, application/ld+json" | ||
| And the header "Allow" should be equal to "OPTIONS, HEAD, GET, POST, DELETE" |
There was a problem hiding this comment.
Shouldn't the value be different for item and collection URLs?
Also, Allow-Post should only be defined for URLs where POST is enabled (usually collections, but not always).
There was a problem hiding this comment.
You’re right, I overlooked that. I’ll make the changes and push them soon.
| $headers['Accept-Patch'] = $acceptPatch; | ||
| } | ||
|
|
||
| $headers['Accept-Post'] = 'text/turtle, application/ld+json'; |
There was a problem hiding this comment.
This value cannot be hardcoded. For instance Turtle is not supported natively by API Platform and JSON-LD may or may not depending on the config.
You should inject the enabled formats here. You can retrieve this from metadata. Listing all allowed media types (not only Turtle and JSON-LD) is probably OK (and a best practice).
There was a problem hiding this comment.
It sounds much better to list all the allowed media types. I'll take care of it.
|
|
||
| private function getAllowedMethods(?string $resourceClass): string | ||
| { | ||
| $allowedMethods = self::DEFAULT_ALLOWED_METHOD; |
There was a problem hiding this comment.
You should filter here the operations with a different URI Template than the one of the current operation to fix the issue described in my 1st comment.
There was a problem hiding this comment.
Ah, good point—I’ll sort this out.
| $headers['Accept-Patch'] = $acceptPatch; | ||
| } | ||
|
|
||
| if (!$exception) { |
There was a problem hiding this comment.
To avoid returning incorrect values—such as in the case of a 401 error when requesting a resource—I added this condition, although I’m not entirely sure it’s the best approach. The issue arises because, in the case of a 401 error, the allowed methods and the currentUriTemplate correspond to the error itself rather than the requested resource.
| } | ||
| } | ||
|
|
||
| return array_unique($allowedMethods); |
There was a problem hiding this comment.
I don't think it's necessary because it's not possible to register two different routes for the same URL and the same method.
It would remove this as we are on the hot path.
To be honest, I would inline this code and remove this method. This will also allow you to set a variable (instead of running an O(n) operation) to check if POST is defined or not for Accept-Post, and this would remove a function call.
(Yes, these are mostly a micro optimization, but this also simplify the code IMHO).
| private function getAllowedMethods(?string $resourceClass, ?string $currentUriTemplate): array | ||
| { | ||
| $allowedMethods = self::DEFAULT_ALLOWED_METHOD; | ||
| if (null !== $currentUriTemplate && null !== $resourceClass && $this->resourceClassResolver->isResourceClass($resourceClass)) { |
There was a problem hiding this comment.
| if (null !== $currentUriTemplate && null !== $resourceClass && $this->resourceClassResolver->isResourceClass($resourceClass)) { | |
| if (null !== $currentUriTemplate && null !== $resourceClass && $this->resourceClassResolver?->isResourceClass($resourceClass)) { |
| { | ||
| $allowedMethods = self::DEFAULT_ALLOWED_METHOD; | ||
| if (null !== $currentUriTemplate && null !== $resourceClass && $this->resourceClassResolver->isResourceClass($resourceClass)) { | ||
| $resourceMetadataCollection = $this->resourceCollectionMetadataFactory ? $this->resourceCollectionMetadataFactory->create($resourceClass) : new ResourceMetadataCollection($resourceClass); |
There was a problem hiding this comment.
| $resourceMetadataCollection = $this->resourceCollectionMetadataFactory ? $this->resourceCollectionMetadataFactory->create($resourceClass) : new ResourceMetadataCollection($resourceClass); | |
| $resourceMetadataCollection = $this->resourceCollectionMetadataFactory ? $this->resourceCollectionMetadataFactory->create($resourceClass) : new ResourceMetadataCollection($resourceClass); |
Better not set the extra headers at all if the factory is missing.
It's an optional feature after all.
| } | ||
| $headers['Allow'] = implode(', ', $allowedMethods); | ||
|
|
||
| if ($isPostAllowed && \is_array($outputFormats = ($outputFormats = $operation->getOutputFormats())) && [] !== $outputFormats) { |
There was a problem hiding this comment.
| if ($isPostAllowed && \is_array($outputFormats = ($outputFormats = $operation->getOutputFormats())) && [] !== $outputFormats) { | |
| if ($isPostAllowed && \is_array($outputFormats = $operation->getOutputFormats()) && [] !== $outputFormats) { |
| private ?IriConverterInterface $iriConverter = null, | ||
| private readonly ?ResourceClassResolverInterface $resourceClassResolver = null, | ||
| private readonly ?OperationMetadataFactoryInterface $operationMetadataFactory = null, | ||
| private readonly ?ResourceMetadataCollectionFactoryInterface $resourceCollectionMetadataFactory = null, |
There was a problem hiding this comment.
We could probably create an LinkedDataPlatformProcessor?
There was a problem hiding this comment.
Thanks for the suggestion! A LinkedDataPlatformProcessor sounds like a good idea, I'll consider implementing it.
| $headers['Accept-Patch'] = $acceptPatch; | ||
| } | ||
|
|
||
| if (!$exception) { |
There was a problem hiding this comment.
if ($operation instanceof ApiPlatform\Metadata\Error)
| foreach ($resource->getOperations() as $resourceOperation) { | ||
| if ($resourceOperation->getUriTemplate() === $currentUriTemplate) { | ||
| $allowedMethods[] = $operationMethod = $resourceOperation->getMethod(); | ||
| $isPostAllowed = $isPostAllowed || ('POST' === $operationMethod); |
There was a problem hiding this comment.
you could collect outputFormats here directly? The operation below may not be the one where the formats are defined.
There was a problem hiding this comment.
Good catch. Thanks.
| } | ||
| $headers['Allow'] = implode(', ', $allowedMethods); | ||
|
|
||
| if ($isPostAllowed && \is_array($outputFormats = ($outputFormats = $operation->getOutputFormats())) && [] !== $outputFormats) { |
There was a problem hiding this comment.
| if ($isPostAllowed && \is_array($outputFormats = ($outputFormats = $operation->getOutputFormats())) && [] !== $outputFormats) { | |
| if ($isPostAllowed && ($outputFormats = $operation->getOutputFormats())) { |
| use Symfony\Component\HttpFoundation\Request; | ||
| use Symfony\Component\HttpFoundation\Response; | ||
|
|
||
| class RespondProcessorTest extends TestCase |
There was a problem hiding this comment.
I think a RespondProcessorTest already exists it must be in tests forgot to move it
There was a problem hiding this comment.
OK, i will merge them.
There was a problem hiding this comment.
The existing RespondProcessorTest use the old way of doing test with prophecy instead of PHPunit mock system, should i refactor it ?
| <argument type="service" id="api_platform.iri_converter" /> | ||
| <argument type="service" id="api_platform.resource_class_resolver" /> | ||
| <argument type="service" id="api_platform.metadata.operation.metadata_factory" /> | ||
| <argument type="service" id="api_platform.metadata.resource.metadata_collection_factory" /> |
tests/Fixtures/TestBundle/Document/DummyGetPostDeleteOperation.php
Outdated
Show resolved
Hide resolved
LaurentHuzard
force-pushed
the
feat/header-for-ldp-spec
branch
from
6619485 to
d072fd5
Compare
LaurentHuzard
force-pushed
the
feat/header-for-ldp-spec
branch
from
d072fd5 to
b6403aa
Compare
soyuka
left a comment
soyuka
left a comment
There was a problem hiding this comment.
nice a few small changes, also you should consider rebasing to be sure that the base ci is green
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
LaurentHuzard
force-pushed
the
feat/header-for-ldp-spec
branch
2 times, most recently
from
f5fd9bf to
64a0bfe
Compare
LaurentHuzard
force-pushed
the
feat/header-for-ldp-spec
branch
2 times, most recently
from
ef08582 to
55fb914
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds HTTP headers to comply with the Linked Data Platform (LDP) specification. The implementation introduces the LinkedDataPlatformProcessor which adds Accept-Post and Allow headers to API responses based on the available operations for each resource.
Changes:
- Added
LinkedDataPlatformProcessorto setAccept-Postheader (listing supported POST formats) andAllowheader (listing permitted HTTP methods) based on resource operations - Integrated the processor into both Symfony and Laravel service configurations
- Created comprehensive test coverage for the new functionality
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/State/Processor/LinkedDataPlatformProcessor.php | Core processor implementation that inspects resource operations and sets LDP-compliant headers |
| src/Symfony/Bundle/Resources/config/symfony/events.php | Symfony service configuration for the LinkedDataPlatformProcessor |
| src/Symfony/Bundle/Resources/config/state/processor.php | Additional Symfony processor service definition with different decoration priority |
| src/Laravel/ApiPlatformProvider.php | Laravel service provider integration for LinkedDataPlatformProcessor |
| tests/Functional/LinkDataPlatformTest.php | Functional tests verifying headers in real HTTP responses |
| src/State/Tests/Processor/LinkedDataPlatformProcessorTest.php | Unit tests for the processor logic |
| tests/Fixtures/TestBundle/ApiResource/DummyGetPostDeleteOperation.php | Test fixture resource with specific operations for testing |
| src/State/Tests/Fixtures/ApiResource/Dummy.php | Minimal test fixture for unit testing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dunglas
left a comment
dunglas
left a comment
There was a problem hiding this comment.
LGTM.
I just wonder if adding new class is it worth it. Maybe this can be added to an existing base processor?
soyuka
force-pushed
the
feat/header-for-ldp-spec
branch
3 times, most recently
from
07bd710 to
abdce50
Compare
To ensure compliance with the LDP specification (https://www.w3.org/TR/ldp/): Added the "Accept-Post" header containing the list of supported Post formats. Added the "Allow" header with values based on the allowed operations on the queried resources.
soyuka
force-pushed
the
feat/header-for-ldp-spec
branch
from
abdce50 to
5eca1f9
Compare
|
thanks! |
3 participants
To ensure compliance with the LDP specification (https://www.w3.org/TR/ldp/):